Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement headless LSP mode #488

Merged

Conversation

ryanabx
Copy link
Contributor

@ryanabx ryanabx commented Aug 25, 2023

Implements godotengine/godot-proposals#7558.

The code adds a new config property, runAtStartup, which is an enum:

none - Default value, the normal behavior users of the plugin expect.
editor - Automatically opens the editor whenever the extension would normally start the connection to the GDScript language server
lsp3, lsp4 - Automatically opens the language server whenever the extension would normally start the connection to the GDScript language server.

The code also adds a button to the error when a GDScript language server cannot be found, called "Run Automatically". Pressing this button will provide the options shown above to the user with an explanation of what they are, then restart the connection process.

@DaelonSuzuka
Copy link
Collaborator

That's... actually a great idea, thanks for the PR!

I'll try to make some time to review it this weekend.

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 14, 2023

That's... actually a great idea, thanks for the PR!

I'll try to make some time to review it this weekend.

Oh, hey! Long time no see, I've been following your work in #452, and eagerly awaiting that pr hehe

@DaelonSuzuka
Copy link
Collaborator

Random thought, does the editor have a command line argument to specify the LSP port? If it does, then we can solve some issues that I was considering intractable.

And yeah, the debugger PR has been a huge pain. I'm hoping I make some progress this weekend.

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 14, 2023

Random thought, does the editor have a command line argument to specify the LSP port? If it does, then we can solve some issues that I was considering intractable.

If not, I can see about making a PR to do that.

And yeah, the debugger PR has been a huge pain. I'm hoping I make some progress this weekend.

Good luck!

@DaelonSuzuka
Copy link
Collaborator

Damn, there isn't a command line argument for picking the LSP port. If there was, it would let us solve a BUNCH of issues with the LSP. Currently, it's not really great to have multiple projects open because the extension has no way to know which editor instance it's talking to. This obviously causes a bunch of problems with errors and completions.

If we could specify the port via CLI, then we could skip trying to connect to an open editor at all, and just launch our own headless Godot process with the LSP on a random high port. Each open workspace could get its own LSP that we know is looking at the correct project. It would also mean that if when the LSP crashes, it wouldn't impact the user's open editor, and the extension could just kill and restart it.

This would also let us sidestep the whole 6005/6008 issue, except maybe as a fallback for there not being a Godot path set or something.

If not, I can see about making a PR to do that.

@ryanabx I have a couple ideas for terrible hacks that I want to test before we try getting engine changes committed. I'd strongly prefer to implement this in a way that works with what users already have, instead of forcing them to update.

That said, it's probably an easy item to implement and I don't see why they'd object to it, so don't like me dissuade you from starting.

While you're digging around in there, try taking a look at the LSP itself. There are a number of known issues with the LSP that I haven't been able to work around from the client side, and it would help a lot of people if we could start to make progress on those.

@Calinou
Copy link
Member

Calinou commented Sep 16, 2023

Damn, there isn't a command line argument for picking the LSP port.

We (or you) can add that to the editor 🙂

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 16, 2023

Damn, there isn't a command line argument for picking the LSP port.

We (or you) can add that to the editor 🙂

Doing you one better, we can probably cherry pick it back to 3.x as well, correct?

@Calinou
Copy link
Member

Calinou commented Sep 16, 2023

Doing you one better, we can probably cherry pick it back to 3.x as well, correct?

Sure 🙂

@atirut-w
Copy link

Or we can read the editor configs?

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 17, 2023

Or we can read the editor configs?

We want to manually specify the language server port, regardless of editor setting. This makes it easier for us to connect to the language server without requiring the user to know what port it's running on (the default ports vary from 3.x to 4.x)

@atirut-w
Copy link

Right, and I had a minor brain fart there, too. Settings that are at their default values are not saved, right?

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 17, 2023

Right, and I had a minor brain fart there, too. Settings that are at their default values are not saved, right?

Ah, I see what you mean now, you're saying we should read it from the user directory, or wherever editor settings are saved on the disk.

Default values aren't saved in the editor settings, no. And I'd be hesitant to rely on the user settings anyways. Ideally the only input we want from the user is the path to the godot exe.

@DaelonSuzuka
Copy link
Collaborator

DaelonSuzuka commented Sep 17, 2023

I just built a prototype that writes to the editor settings to try and bypass needing a command line flag for the LSP port. This idea was roughly:

  • open editor settings
  • save a copy
  • change network/language_server/remote_port to a random high port
  • launch a headless Godot process to use as LSP
  • restore original editor settings

Unfortunately, when I restored the original editor settings, the headless Godot noticed the file change, reloaded its settings, and changed its LSP port back to the original. That makes this a dead end.

Another idea was to provide a fake editor settings file to each headless instance, but there's no way to do that either. I don't understand how there's no flag for that because it seems like it would be instrumental in testing the damn editor, but here we are.

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 17, 2023

Unfortunately, when I restored the original editor settings, the headless Godot noticed the file change, reloaded its settings, and changed its LSP port back to the original. That makes this a dead end.

Yeah, I noticed that in the code earlier today. It'll emit a NOTIFICATION_SETTINGS_CHANGED or something like that and the language server hooks into that to check if the port or host changed.

Another idea was to provide a fake editor settings file to each headless instance, but there's no way to do that either. I don't understand how there's no flag for that because it seems like it would be instrumental in testing the damn editor, but here we are.

That's an idea. I was thinking about a command line argument to specify an arbitrary number of overrides to the editor settings but that might be something to consider. It might be easier to specify a path for a separate editor settings file.

EDIT: The alternate editor file should probably be a list of overrides to the general user settings. The reason for this is that some people have certain editor settings changed that they'd want to keep. For instance, I changed my editor's text_editor/completion/idle_parse_delay to 1.0 seconds instead of 2.0

@turbohz
Copy link

turbohz commented Sep 17, 2023

Hi.

I have a setup like this working, and works well. I have a Godot Server with some modifications, one of which implements a "--lsp-port" command line argument.

I'm not very experienced using C++, and I didn't think many people was interested on such feature, so I never tried to get this upstreamed.

Anyway, if anyone is interested, take a look.
turbohz/godot@ad02527

(This is for Godot 3.x)

@DaelonSuzuka
Copy link
Collaborator

EDIT: The alternate editor file should probably be a list of overrides to the general user settings.

From a general purpose/API perspective, there's no real excuse to not have both behaviors (replace and override) available if the user wants them, but for this LSP situation I agree that we just need an additional layer of override.

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 18, 2023

Hi.

I have a setup like this working, and works well. I have a Godot Server with some modifications, one of which implements a "--lsp-port" command line argument.

I'm not very experienced using C++, and I didn't think many people was interested on such feature, so I never tried to get this upstreamed.

Anyway, if anyone is interested, take a look.

Hey! I'll definitely be looking this over and I'll see about making a PR.

I had an in-progress branch on my own godot fork and couldn't think of the best way to override the lsp port, I didn't think about using a static property!

@DaelonSuzuka
Copy link
Collaborator

DaelonSuzuka commented Sep 18, 2023

I made some changes to the user-facing API and simplified the new setting to a bool.

I also refactored the internals to cut down the number of code paths and to not use open_workspace_with_editor() or run_editor(), which are entirely too complicated for what they do.

Things left to do:

  • split godotTools.editorPath into two fields, one for g3 and one for g4
  • totally rebuild the "can't connect to editor" dialog and the resulting workflow. I've been wanting to do this for a while, and I think now's the time.
  • find a better way to manage child processes (there's a comment about this)
  • move most of this code out of the main file and into the lsp module. I'm thinking a new ClientConnectionManager class that owns all the retrying, status checking, the statusbar widget, etc.

@ryanabx
Copy link
Contributor Author

ryanabx commented Sep 18, 2023

I made some changes to the user-facing API and simplified the new setting to a bool.

I also refactored the internals to cut down the number of code paths and to not use open_workspace_with_editor() or run_editor(), which are entirely too complicated for what they do.

Thanks! I'll look over those changes later, at the gym right now :p

Are you active on the godot contributors chat? If not you should be!

@DaelonSuzuka DaelonSuzuka changed the title Add ability to automatically run editor/language server at startup Implement headless LSP mode Oct 11, 2023
@DaelonSuzuka
Copy link
Collaborator

Headless LSP mode works. It has an error handling workflow that will hopefully prevent trying to launch a headless LSP with a wrong/old version of Godot specified in editorPath.godot3/4.

I also snuck in a fix for #373 (bad formatting in hovers), and a feature improvement to allow right clicking on type-hinted variables and opening the godot docs for them. (these were both LSP-adjacent)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants